-
Notifications
You must be signed in to change notification settings - Fork 80
Fix Windows provider CLI spawn + cross-platform scripts #638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix Windows provider CLI spawn + cross-platform scripts #638
Conversation
|
@JKamsker is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex review |
|
@JKamsker - Your PR looks great! I have one suggestion that would make the PTY spawning even more Currently on Unix, when spawning provider terminals, we rely on the shell's PATH to find the CLI: To make this more reliable (especially for users with non-standard shell configs), we could resolve Add this helper function near the top of ptyManager.ts (around line 15): } Then modify the provider command building (around line 182-189): // To: This would make the provider spawning use absolute paths (e.g., What do you think? Happy to test this addition if you'd like to include it! The rest of your PR |
|
lmk what you think @JKamsker :) |
|
Hey @arnestrickmann, thanks for the quick reply! |
- Prefer executable extensions from where on win32 (e.g. .cmd) - Spawn the resolved executable path (and use shell for cmd/bat/ps1) - Replace unix-only npm scripts (rm/cp/mkdir) with node scripts - Make postinstall rebuild native deps with optional PTY rebuild
- Show real PTY start errors instead of 'isn’t installed' - Allow suppressing install command for start failures - Resolve and spawn provider CLIs robustly on Windows
Include PATH/PATHEXT and key Windows env vars in the PTY environment so codex.cmd can locate ode.
0af9715 to
2596e48
Compare
Summary
This PR fixes provider CLI detection/spawning on Windows (e.g.
codex), and makes the dev/build scripts cross-platform so Windows can run the project without relying on Unix shell utilities.What changed
src/main/services/ConnectionsService.tsnow prefers executable extensions fromwhere(e.g..cmd) and spawns the resolved executable path..cmd/.batexecution: on win32, when the resolved executable is.cmd/.bat/.ps1, spawning usesshell: truesospawn()doesn’t fail (e.g.EINVAL).rm -rf,mkdir -p, andcpusage with small Node-based scripts.postinstallnow runs a script that rebuilds required native deps and optionally rebuilds PTY support (skipped when PTY is disabled).Why
where codexcan return an extensionless shim first (e.g.%APPDATA%\npm\codex). Node’sspawn()cannot execute that shim directly, causingspawn ... ENOENTeven though the tool is installed..cmddirectly without a shell can also produceEINVAL.Testing
where codexresolves both shim and.cmd, and the code now selects.cmd.npm run type-check.npm run devon Windows and confirmed providers likecodexshow as connected.Notes
EMDASH_DISABLE_PTY=1for environments lacking the required Visual Studio Spectre-mitigated libs.Note
Improves Windows CLI detection/execution and makes tooling cross‑platform, plus better UI surfacing of terminal start failures.
whereand spawn withshellfor.cmd/.bat/.ps1inConnectionsServiceandPrGenerationService(alsowindowsHide)ptyManager: cleaner environment initialization; add Windows PATH/PATHEXT/SystemRoot vars and better shell resolution/errorsInstallBannergainsstart_failedmode with error details (incl. PTY-disabled hint);ChatInterfacecaptures and displays CLI start errorscopy-app-config.cjs,clean.cjs); newpostinstall.cjsselectively runselectron-rebuildfor native deps (e.g.,sqlite3,node-pty,keytar); updatepackage.jsonscripts accordinglyWritten by Cursor Bugbot for commit 83f319a. This will update automatically on new commits. Configure here.